Better to merge this first

Sure thing, I just didn't want to wait before creating a PR, when the other one gets merged in I'm gonna rebase this one.

Thanks for working on this, @Andarist! Apologies for creating a rebase headache by merging #141.

Dont worry, i knew it will be needed later from the very beginning, also shouldnt be particularly headache-y ;)

Ok, I've rebased this PR, please keep in mind that this is not merge-ready, because of the usage of custom (non es5 compatible) babel-plugin-transform-builtin-extend. If you decide that you want to merge this PR, it should be rewritten first - my proposal is to just create buble-compatible class transform. Ofc I offer to write it 😉

hi @Andarist this is quite interesting setup you have here, is there anyway to compare the results of this setup to our current setup to see what wins we get?

The diff can be seen here. Unfortunately it is not easily comparable. I've studied it for some time and mostly (haha 🤣) the difference is in class transform:

• buble generates a little bit simpler classes
• buble handles built-in extension just fine (you extend Error), atm babel doesnt handle this too well
• buble for simple cases doesnt wrap prototype pattern in an IIFE (it does it only when u use inheritance) - this is not a good thing, assignments to the unused function (such as prototype method assignment to the constructor) prevents dead code elimination
• with babel you can use rich ecosystem of plugins (ive added one for adding #__PURE__ annotations), or write ur own

Ofc as mentioned above - ur original code style is lost in the output when using babel.

any examples of bundle size comparisons ?

Sure - I can check them out and report back later, but diffs will be a little bit off, because a PR is not finished due to the issues described regarding class transform.

I always measure such things by inspecting minified+gziped UMD bundles, is that ok or should I measure in some other way?

I think that would be nice to see you could use https://github.com/mostjs/core/tree/master/examples/counter as a way to compare

I always measure such things by inspecting minified+gziped UMD bundles, is that ok

@Andarist Yep, that'd be just fine. Thank you for all your work on this so far! Looking forward to seeing size comparisons.

Ping @Andarist. We're definitely still interested in seeing a size comparison. Is there anything else we can do to help?

Nah, not really - I will send comparisons as soon as I can, just couldn't focus at this PR lately as I had some other issues to tackle on.

Thanks @Andarist. No worries 😄

All results for UMD minified and gzipped bundles

./packages/core/dist/mostCore.js
pre - 9.27 kB
post - 9.47 kB

./packages/disposable/dist/index.js
pre - 1.59 kB
post - 1.87 kB

./packages/prelude/dist/index.js
pre - 1.06 kB
post - 1.05 kB

./packages/scheduler/dist/index.js
pre - 3.3 kB
post - 3.31 kB

Difference is there, although quite small and as I've written before - alternative class transform should be written before merging this in, which potentially could decrease the those size differences.

I've also noticed that only the core package is building uglified bundle, other do not do that. Is it intended?

@Andarist Whew! Now that 1.0 is released, I'm going to spend some time on this. My plan is to build https://github.com/mostjs/core/tree/master/examples/counter using master and this branch so we can see the size difference in an application. Granted, it's a trivial application, but I think that's a good place to start since it should result in a fairly large size difference (at least, I think it should!). If it does then I think we'll feel good about moving ahead.

Recent versions of rollup might be able to tree-shake some curry calls so actually #__PURE__ might help less there.

The experiment should use webpack with ModuleConcatenationPlugin on and uglifyjs, those settings should give better tree-shaking results - webpack alone actually has quite poor algorithm for this stuff.

I will try to rebase the branch a little bit later.

I made a couple local branches, one based on this PR branch (rebased to latest master), and one based on latest master. Both add rollup-plugin-uglify and uglify-es to the examples dir rollup config. I built the counter app in both branches, and verified that both still work correctly in Chrome, with no errors or warnings in the console.

Here are the results with just uglify, and with uglify+gzip:

❯ /bin/ls -l
total 168
-rw-r--r--  1 brian  staff  38834 Nov 28 08:13 index.base.js
-rw-r--r--  1 brian  staff   7792 Nov 28 08:24 index.base.js.gz
-rw-r--r--  1 brian  staff     89 Nov 28 08:18 index.js.map
-rw-r--r--  1 brian  staff  21925 Nov 28 08:18 index.pure.js
-rw-r--r--  1 brian  staff   4581 Nov 28 08:24 index.pure.js.gz

index.base.* is current master without PURE annotations, and index.pure.* is with PURE annotations. The non-gzip pure version is 21925 / 38834 = 56% of the size of the corresponding base version. The gzip pure version is 4581 / 7792 = 58% the size of the corresponding base version.

The savings are quite significant. Again, this is for a trivial app that uses a tiny amount of the total API surface area. Obviously, an app that uses more of the API will see less of a savings ... but that's the whole point: You get exactly what you opt into, and no more.

This seems compelling enough to me to proceed, but I'd love to hear everyone else's thoughts.

@Andarist Ok, thanks for the info about recent rollup. I'll try to run the experiment again with the latest version and post more results. We should do a similar test with webpack. The savings are probably less, but its worth seeing.

I have to switch to day job mode now, but I'll push my branches when I have a chance in case anyone else wants to experiment.

And there we go.

Next, I'm going to try to get rid of the need for extending Error. I'll do that in another PR so we can all see what it looks like before making a decision about it.

👍 that would be a breaking change though, wouldnt it?

I think it depends on exactly what we do. The particular error thrown by disposeAll isn't part of the public API, so I feel it's ok to change it. Using native prototypal inheritance instead of extends may be a solution as well.

Then both options seems fine to me, certainly better to use prototypal inheritance for a single use case than adding an additional (somewhat) complex plugin. If you follow that path it's advised to use IIFE to output a class into a single variable.

For comparison:

1. 85031a7 uses prototypal inheritance. This seems pretty good to me.
2. 7bcb378 uses a regular Error. This is slightly a "bigger" change. I don't see much advantage, but it also seems fine to me.

Personally I like 1st better, seems like a cleaner change and being more straightforward about its purpose.

I'd only change it to:

export const DisposeAllError = (Error => {
  function DisposeAllError(message, errors) {
    Error.call(this, message)
    this.message = message
    this.name = DisposeAllError.name
    this.errors = errors

    if (Error.captureStackTrace) {
      Error.captureStackTrace(this, DisposeAllError)
    }

    this.stack = `${this.stack}${formatErrorStacks(this.errors)}`
  }	

  DisposeAllError.prototype = Object.create(Error.prototype)

  return DisposeAllError
})(Error) 

@Andarist Agreed about the first option.

What's the reason for the IIFE in this particular case? It's not clear to me what it's defending against.

Having this in top level scope:

DisposeAllError.prototype = Object.create(Error.prototype)

will prevent tree-shaking of DisposeAllError in most cases. Because assignments like this are treated as effectful, especially considering the fact that Object.create may not be whitelisted as "pure" in dead code eliminators like uglifyJS (it might be, aint sure about this, would have to check).

will prevent tree-shaking of DisposeAllError in most cases

Ah, ok, thanks for explaining. I'll add the IIFE.

Ok, there we go: 1019bda

Do you want me to cherry pick that onto this branch, and remove the babel plugin?

Yeah 👍 that would simplify working with this branch :)

Done! Cherry-picked prototypal inheritance and removed builtin-extend plugin on this branch.

Cool! This PR is ready to merge then, if you want to proceed with it. Let me know if it needs any other adjustments.

Here are results with webpack:

❯ /bin/ls -lrt counter/dist/
total 184
-rw-r--r--  1 brian  staff  25121 Dec  7 08:42 index.webpack.pure.js
-rw-r--r--  1 brian  staff   5481 Dec  7 08:44 index.webpack.pure.js.gz
-rw-r--r--  1 brian  staff  42553 Dec  7 08:55 index.webpack.base.js
-rw-r--r--  1 brian  staff   9486 Dec  7 08:56 index.webpack.base.js.gz

25121 / 42553 = ~59% of original non-gzip
5481 / 9486 = ~58% of original gzip

And here's the webpack config I used:

const path = require('path')
const webpack = require('webpack')
const UglifyJSPlugin = require('uglifyjs-webpack-plugin')

module.exports = {
  entry: './counter/src/index.js',
  output: {
    filename: 'index.js',
    path: path.resolve(__dirname, 'counter/dist')
  },
  resolve: {
    mainFields: [ 'module', 'jsnext:main', 'browser', 'main' ]
  },
  devtool: 'source-map',
  plugins: [
    new webpack.optimize.ModuleConcatenationPlugin(),
    new UglifyJSPlugin()
  ]
}

I pushed the two branches I used for the tests. Be sure to npm install in both the top level dir and the examples dir. You may also want to verify that for the PURE test, you have indeed ended up with PURE annotations in examples/node_modules/@most/core/dist/index.es.js. When I did the test the first time, somehow I missed a step and ended up without them--thus making it appear as if there was no savings at all.

test without PURE: https://github.com/mostjs/core/tree/x-webpack-tree-shake-master
test with PURE: https://github.com/mostjs/core/tree/x-webpack-tree-shake-pure

Cool comparisons! Would be good to test webpack without ModuleConcatenationPlugin too, I predict though that it won't give even nearly similar savings - should save a little bit of something though. Good news is that webpack@4 is gonna have ModuleConcatenationPlugin enabled by default for production builds.

Without ModuleConcatenationPlugin (See index.webpack.pure-no-concat.*), surprisingly similar results. It may be because this is a trivial application, though.

❯ /bin/ls -lr counter/dist/
total 256
-rw-r--r--  1 brian  staff   5481 Dec  7 08:44 index.webpack.pure.js.gz
-rw-r--r--  1 brian  staff  25121 Dec  7 08:42 index.webpack.pure.js
-rw-r--r--  1 brian  staff   5655 Dec  7 12:05 index.webpack.pure-no-concat.js.gz
-rw-r--r--  1 brian  staff  26122 Dec  7 12:04 index.webpack.pure-no-concat.js
-rw-r--r--  1 brian  staff   9486 Dec  7 08:56 index.webpack.base.js.gz
-rw-r--r--  1 brian  staff  42553 Dec  7 08:55 index.webpack.base.js

webpack config:

const path = require('path')
// const webpack = require('webpack')
const UglifyJSPlugin = require('uglifyjs-webpack-plugin')

module.exports = {
  entry: './counter/src/index.js',
  output: {
    filename: 'index.js',
    path: path.resolve(__dirname, 'counter/dist')
  },
  resolve: {
    mainFields: [ 'module', 'jsnext:main', 'browser', 'main' ]
  },
  devtool: 'source-map',
  plugins: [
    // new webpack.optimize.ModuleConcatenationPlugin(),
    new UglifyJSPlugin()
  ]
}

@Andarist Huge thanks for starting the conversation in #138 and for this PR. @TrySound, thank you as well for the help and discussion.

I'm having a real "this is why open source is awesome" moment right now about the awesome collaboration 😄 ... and I'm excited that we'll be able to release such a concrete improvement for @most/core users!

I'm gonna go through the diff one last time to make sure we've tied up all loose ends.

Actually results are similar because you are producing a flat bundle 👏

If you'd transpile file by file and produce a "mirror" directory structure results would be a lot worse probably and that would be exposed even more by inspecting a "trivial app" because in such a trivial app tree shaking should do more than for more complex app which would probably use a lot more of the core and thus benefits would be smaller.

I'm having a real "this is why open source is awesome" moment right now about the awesome collaboration 😄 ... and I'm excited that we'll be able to release such a concrete improvement for @most/core users!

Thanks for kind words :) I have just explored this area quite a bit lately and observed what prevents and what not tree shaking algos, so I've looked on several libs and tried to improve the situation where I could

Just published @most/core 1.1.0 with PURE annotations 🚀